fix(st-8nn): arm the out-of-band snapshot gate, and key it on content - #113
Merged
Conversation
The gate the server's snapshot cache was supposed to run (st-69h) has never fired in production. Three separate faults, each hidden by the next: 1. publishList — the ONLY publish path a request takes — built its snapshot without reading the store signal at all. putList stamps it correctly but has no non-test caller. Every request-path snapshot therefore carried a zero stamp, and both freshEntryLocked and checkStale skip an unstamped entry. Net effect: strand served a stale board for the life of the process after any bd-CLI or fleet-agent write. 2. The signal was the manifest MTIME, which a pure bd read rewrites with byte-identical content. Arming the gate on mtime would have swapped one bug for its mirror image — the server evicting the snapshot it just published on its own reads, the self-churn internal/counts already diagnosed and fixed by moving to a content key (st-3wp.1). So this had to move with the fix, not after it. 3. handlePulse's doc comment describes calling checkStale and kicking a background rebuild (st-2fy.5). The body did neither, and checkStale had no production caller. Fix: publishList stamps the key on both branches (a byte-identical list still takes the fresh key — it was just re-validated against bd, and keeping a stale key would make the very next read evict a snapshot just confirmed good). bd.StoreContentKey replaces bd.StoreMTime behind the seam, so equality replaces ordering — which also catches a rollback that mtime cannot. handlePulse wires the rebuild its comment promised, after the render and detached from the request ctx. storeKeyOK is carried alongside the key because 0 is a legal hash value and cannot double as "unset"; an unreadable store still degrades to serve-until- invalidated, never falsely stale. Two of the three tests over this gate passed vacuously — TestSnapshotCache- StoreMTimeGate asserted ONE spawn after the store moved and read that as "filesystem signals are not authoritative", when the real cause was that nothing could ever evict. They now assert eviction. Added TestSnapshotCacheStoreKeyStampedOnPublish as the direct regression guard on fault 1, and a restamp test covering publishList's unchanged-list branch. make check green; -race green on internal/server. Closes st-8nn
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
st-8nnfiled this as "the server cache self-churns on its own reads, same disease counts had." Investigating it turned up the opposite failure and two more alongside it — the gate has never fired in production at all.What was actually broken
1. The gate was never armed.
publishList— the only publish path a request takes — built its snapshot without reading the store signal.putListstamps it correctly but has no non-test caller. BothfreshEntryLockedandcheckStaleskip an unstamped entry, so strand served a stale board for the life of the process after any bd-CLI or fleet-agent write. Probed directly before fixing:storeAtwas zero after a realcachingSource.List.2. The signal was mtime. That is st-8nn's original point and it stands — a pure bd read rewrites the manifest with byte-identical content. Arming the gate on mtime would have swapped one bug for its mirror image: the server evicting the snapshot it just published, which is the self-churn
internal/countsfixed in sd-3wp.1 by moving to a content key. The two changes had to land together.3.
handlePulsenever did what its comment says. The doc describes callingcheckStaleand kicking a background rebuild (st-2fy.5); the body only rendered.checkStalehad no production caller.Fix
publishListstamps the key on both branches. The unchanged-list branch restamps too — the list was just re-validated against bd, so keeping the old key would make the very next read evict a snapshot just confirmed good.bd.StoreContentKeyreplacesbd.StoreMTimebehind the seam. Equality replaces ordering, which also catches a rollback (bd dolt reset) that mtime cannot.handlePulsekicks the rebuild after the render, detached from the request ctx like handleHome's deps prefetch.storeKeyOKrides alongside the key because 0 is a legal hash value and cannot double as "unset".Tests
Two of the three tests over this gate passed vacuously.
TestSnapshotCacheStoreMTimeGateasserted one spawn after the store moved and explained it as "filesystem signals are not authoritative" — the real cause was that nothing could ever evict. They now assert eviction. New:TestSnapshotCacheStoreKeyStampedOnPublishguards fault 1 directly, and a restamp test coverspublishList's unchanged-list branch.make checkgreen,-racegreen on internal/server.Closes st-8nn